-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update shader bencher to share some logic with snapshots #8108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Update shader bencher to share some logic with snapshots #8108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm this is a tough one. It's almost like we should have a new crate for naga tests, which wgpu-benchmark can depend on.
I'll bring this up at the next meeting as we had a whole discussion about the placement of naga test and I'm not sure how this new issue would change opinion.
walkdir.workspace = true | ||
wgpu-test.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty uncomfortable making this back edge - I feel like there's a better design for sharing the benchmarks. More specifically I'm not really sure why this backedge needs to be here at all if the tests have been moved to wgpu-test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting that this needs discussion before landing
Ok. I probably won't be able to be there but I don't think I'd be of use in those discussions anyway. And its fine if this PR gets closed, I made these changes to stop a CI from failing which it already accomplished. |
Don't you still need it to land your changes?
We'll keep you in the loop, no worries |
I will need it for whenever mesh shader related tests get added. I'd really really like to get the tests in with WGSL parsing but if that has to come later its not my problem (and the testing has already been done on those changes). |
Alright, I'll see if we can muse up a better solution as we definitely want this problem to be solved when mesh shader wgsl-in lands. |
Connections
This is to address test failings in #7930 due to not respecting supported backends. In fact, its pulled directly from the last few commits there.
Description
Previously, the shader bencher was using snapshot shaders but not respecting their options, such as which backends they should be tested against. As a result, in #7930, a mesh shader test shader that only can be written to SPIRV was causing the shader bencher to fail. I have updated the shader bencher and snapshots to share more logic.
See this comment
Testing
The change is to testing itself. I have verified that the bencher still fails on bad shader inputs, and that it ignores my mesh shader as it is supposed to.
Squash or Rebase?
Squash
Checklist
cargo fmt
.taplo format
.cargo clippy --tests
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.CHANGELOG.md
entry.